feat: add experimental ACP mode (--experimental-acp)#186
feat: add experimental ACP mode (--experimental-acp)#186johnstcn wants to merge 4 commits intocj/refactor/event-emitterfrom
Conversation
|
✅ Preview binaries are ready! To test with modules: |
Add support for Agent Control Protocol (ACP) as an alternative to terminal emulation. ACP uses JSON-RPC over stdin/stdout pipes. - Introduce AgentIO interface to abstract PTY vs ACP transports - Add ACPConversation implementing Conversation interface - Add --experimental-acp flag (mutually exclusive with --print-openapi) - Add e2e test with mock ACP agent - Block `attach` when using --experimental-acp (no terminal) - Update chat UI to show ACP tool calls Other changes: - chat: Fix redundant draft filtering from finally block Created using Mux (Opus 4.5)
| setMessages((prevMessages) => | ||
| prevMessages.filter((m) => !isDraftMessage(m)) | ||
| ); |
There was a problem hiding this comment.
review: this was causing a 'flicker' when sending a message in the UI
| } | ||
|
|
||
| if status.ACPMode { | ||
| return xerrors.New("attach is not supported in ACP mode. The server is running with --experimental-acp which uses JSON-RPC instead of terminal emulation.") |
There was a problem hiding this comment.
review: It should be eventually supported but I'm not sure what form it should take yet.
| func (c *acpClient) RequestPermission(ctx context.Context, params acp.RequestPermissionRequest) (acp.RequestPermissionResponse, error) { | ||
| // Auto-approve all permissions for Phase 1 | ||
| return acp.RequestPermissionResponse{ | ||
| Outcome: acp.RequestPermissionOutcome{ | ||
| Selected: &acp.RequestPermissionOutcomeSelected{OptionId: "allow"}, | ||
| }, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
review: we need to add support for allow/deny; will be handled in a future PR
There was a problem hiding this comment.
Pull request overview
This PR adds experimental support for Agent Control Protocol (ACP) as an alternative transport to PTY-based terminal emulation. ACP uses JSON-RPC over stdin/stdout pipes for cleaner communication without terminal escape sequences.
Changes:
- Introduces
AgentIOinterface abstraction for PTY vs ACP transports - Implements
ACPConversationandACPAgentIOfor ACP protocol support - Adds
--experimental-acpCLI flag with validation against--print-openapi - Adds Backend field to status API response and blocks attach command in ACP mode
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| x/acpio/acpio.go | Core ACP I/O implementation with JSON-RPC connection handling |
| x/acpio/acp_conversation.go | ACP conversation tracker with async message handling |
| x/acpio/acp_conversation_test.go | Comprehensive unit tests for ACP conversation |
| lib/httpapi/setup.go | ACP process setup and lifecycle management |
| lib/httpapi/server.go | Server integration with transport abstraction |
| lib/httpapi/server_test.go | Updated tests to use AgentIO interface |
| lib/httpapi/models.go | Added Backend field to status response |
| cmd/server/server.go | CLI flag and transport selection logic |
| cmd/attach/attach.go | ACP mode detection and rejection for attach command |
| openapi.json | API schema update with backend field |
| go.mod/go.sum | Added acp-go-sdk dependency |
| lib/acp/doc.go | Package documentation for ACP support |
| e2e/acp_echo.go | Mock ACP agent for e2e testing |
| e2e/echo_test.go | E2E test for ACP mode |
| e2e/testdata/acp_basic.json | Test script data |
| chat/src/components/chat-provider.tsx | Removed draft message cleanup (appears to be a bug) |
Comments suppressed due to low confidence (1)
lib/httpapi/server_test.go:37
- The tests don't set the Transport field in ServerConfig, which means it will be an empty string. The server code will default to PTY transport when config.Transport != "acp", but the Backend field in the status response will be an empty string instead of "pty". This could cause issues for clients that check the backend field.
Consider setting a default value "pty" for config.Transport when it's empty, or explicitly setting Transport in all tests.
srv, err := httpapi.NewServer(ctx, httpapi.ServerConfig{
AgentType: msgfmt.AgentTypeClaude,
AgentIO: nil,
Port: 0,
ChatBasePath: "/chat",
AllowedHosts: []string{"*"},
AllowedOrigins: []string{"*"},
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| go func() { | ||
| select { | ||
| case <-ctx.Done(): | ||
| logger.Info("Context done, closing ACP agent") | ||
| _ = stdin.Close() | ||
| _ = stdout.Close() | ||
| // Try graceful shutdown first | ||
| _ = cmd.Process.Signal(syscall.SIGTERM) | ||
| // Force kill after timeout | ||
| time.AfterFunc(5*time.Second, func() { | ||
| _ = cmd.Process.Kill() | ||
| }) | ||
| case <-done: | ||
| // Process exited normally, nothing to clean up | ||
| return | ||
| } | ||
| }() |
There was a problem hiding this comment.
The cleanup goroutine in SetupACP has a potential race condition with the AfterFunc timer. If the context is cancelled, SIGTERM is sent, and then exactly 5 seconds later the cleanup goroutine receives from the done channel (normal exit), the AfterFunc will still fire and call Kill() on an already-exited process.
Consider using a sync mechanism to cancel the AfterFunc timer when the process exits normally, or checking if the process has exited before calling Kill().
| if (type === "user") { | ||
| setMessages((prevMessages) => | ||
| prevMessages.filter((m) => !isDraftMessage(m)) | ||
| ); | ||
| setLoading(false); | ||
| } | ||
| } |
There was a problem hiding this comment.
The removed lines filtered out draft messages before setting loading to false. Without this cleanup, draft messages (messages with undefined id) could remain in the messages array if an error occurs during the POST request. This could cause UI inconsistencies where draft/optimistic messages persist even though the actual message was never sent.
The draft message cleanup should be performed in the finally block (or elsewhere) to ensure consistency.
| case <-ctx.Done(): | ||
| logger.Info("Context done, closing ACP agent") | ||
| _ = stdin.Close() | ||
| _ = stdout.Close() | ||
| // Try graceful shutdown first | ||
| _ = cmd.Process.Signal(syscall.SIGTERM) | ||
| // Force kill after timeout | ||
| time.AfterFunc(5*time.Second, func() { | ||
| _ = cmd.Process.Kill() | ||
| }) |
There was a problem hiding this comment.
When the context is cancelled, the cleanup goroutine closes stdin/stdout pipes and signals SIGTERM to the process. However, the main goroutine (lines 185-194) is simultaneously calling acpResult.Wait() which calls cmd.Wait(). Closing the pipes while the ACP connection might still be active could cause the ACP SDK to encounter unexpected EOF errors or other I/O errors.
Consider coordinating the shutdown sequence so that the ACP connection can be gracefully closed before the pipes are closed, or ensure that the ACP SDK properly handles pipe closure during context cancellation.
| // Remove the empty streaming message on error | ||
| if len(c.messages) > 0 && c.messages[len(c.messages)-1].Role == st.ConversationRoleAgent && | ||
| c.messages[len(c.messages)-1].Message == "" { |
There was a problem hiding this comment.
When an error occurs, the code checks if the last message is an empty agent message before removing it. However, the check only verifies Message == "". If the agent has started streaming chunks before an error occurs, the message won't be empty and won't be removed, leaving a partial/incomplete agent message in the history.
Consider either removing the agent message regardless of its content on error, or marking it with an error state so the UI can indicate the message was incomplete.
| // Remove the empty streaming message on error | |
| if len(c.messages) > 0 && c.messages[len(c.messages)-1].Role == st.ConversationRoleAgent && | |
| c.messages[len(c.messages)-1].Message == "" { | |
| // Remove the in-progress streaming agent message on error | |
| if len(c.messages) > 0 && c.messages[len(c.messages)-1].Role == st.ConversationRoleAgent { |
| "textLen", len(text), | ||
| "rawDataLen", len(data)) | ||
|
|
||
| // Use a timeout to prevent hanging indefinitely |
There was a problem hiding this comment.
The ACPAgentIO stores the context passed to NewWithPipes and uses it later in Write() for creating the prompt timeout context. If the parent context is cancelled (e.g., server shutdown), any in-flight Write() call will use context.WithTimeout on an already-cancelled context, which will immediately return a cancelled error instead of the intended timeout behavior.
Consider checking if the stored context is already cancelled before using it, or document that Write() may fail with context.Canceled during shutdown.
| // Use a timeout to prevent hanging indefinitely | |
| // Use a timeout to prevent hanging indefinitely | |
| if err := a.ctx.Err(); err != nil { | |
| a.logger.Debug("Agent context already canceled, aborting prompt", "error", err) | |
| return 0, err | |
| } |
1 participant
Depends on #185
Add support for Agent Control Protocol (ACP) as an alternative to terminal emulation. ACP uses JSON-RPC over stdin/stdout pipes.
Created using Mux (Opus 4.5)